-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPES-1549: Allow to delete users #206
base: master
Are you sure you want to change the base?
Conversation
4b81d7f
to
3b80ff9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of feedback.
I want to submit this already, but I still want to have a closer look at the new tests.
b671bea
to
6a3a844
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some changes suggested, but not hard blockers.
Some points we missed in our latest planning. Conflict with default methodIf we enable "Restrict options" setting, but set the default cancel method to something that deletes, then:
Interesting find in $form['registration_cancellation']['user_cancel_method'] += user_cancel_methods();
foreach (Element::children($form['registration_cancellation']['user_cancel_method']) as $key) {
// All account cancellation methods that specify #access cannot be
// configured as default method.
// @see hook_user_cancel_methods_alter()
if (isset($form['registration_cancellation']['user_cancel_method'][$key]['#access'])) {
$form['registration_cancellation']['user_cancel_method'][$key]['#access'] = FALSE;
}
} See the comment about 'access' key. Right now / with this PR, the available options in the account settings form are dependent on your user account, that is, whether you are uid 1. I think this entire problem is not a regression, so we could move ahead and ignore it for now. Which form to useFrom UX perspective it seems this setting belongs into "Account settings" form, not "Authentication settings". TBD with rest of the team. |
As per #79 (comment), it's for the site developers to configure a proper default method when restricting the options. Since the module behaves already in that way, we won't add anything as part of this pull request. We could iterate in a different one. |
oe_authentication.module
Outdated
$current_user = \Drupal::currentUser(); | ||
$restrict_user_delete = \Drupal::configFactory() | ||
->get('oe_authentication.settings') | ||
->get('restrict_user_delete_cancel_methods'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We forgot to add a fallback to TRUE, just for the sake of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we don't, and rely on upgrade path?
I don't mind if we add it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know we went back and forth a lot on this. I first said no, then I said ok to add it just to avoid any disruption.
And I should see "Delete the account and make its content belong to the Anonymous user." | ||
|
||
Scenario: Users should be able to delete other users when the configuration to | ||
restrict the user delete options is not enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't split this
6a3a844
to
1791d41
Compare
Fixes #184